From 32f049458805d970d5cf2b5a1431e8231e0f038d Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Mon, 10 Nov 2025 19:27:51 -0300 Subject: [PATCH] [PATCH] lib,permission: require full read and write to symlink APIs Refs: https://hackerone.com/reports/3417819 PR-URL: https://github.com/nodejs-private/node-private/pull/760 Reviewed-By: Matteo Collina CVE-ID: CVE-2025-55130 Signed-off-by: RafaelGSS Gbp-Pq: Topic sec Gbp-Pq: Name 36-lib-permission-require-full-read-and-write-to-symlink-apis.patch --- lib/fs.js | 34 ++++++------------- lib/internal/fs/promises.js | 20 +++-------- .../permission/fs-symlink-target-write.js | 18 ++-------- test/fixtures/permission/fs-symlink.js | 18 ++++++++-- .../test-permission-fs-symlink-relative.js | 10 +++--- test/parallel/test-permission-fs-symlink.js | 14 ++++++++ 6 files changed, 52 insertions(+), 62 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 0ee3ec590..655f14c30 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -59,7 +59,6 @@ const { } = constants; const pathModule = require('path'); -const { isAbsolute } = pathModule; const { isArrayBufferView } = require('internal/util/types'); const binding = internalBinding('fs'); @@ -1745,18 +1744,12 @@ function symlink(target, path, type_, callback_) { const type = (typeof type_ === 'string' ? type_ : null); const callback = makeCallback(arguments[arguments.length - 1]); - if (permission.isEnabled()) { - // The permission model's security guarantees fall apart in the presence of - // relative symbolic links. Thus, we have to prevent their creation. - if (BufferIsBuffer(target)) { - if (!isAbsolute(BufferToString(target))) { - callback(new ERR_ACCESS_DENIED('relative symbolic link target')); - return; - } - } else if (typeof target !== 'string' || !isAbsolute(toPathIfFileURL(target))) { - callback(new ERR_ACCESS_DENIED('relative symbolic link target')); - return; - } + // Due to the nature of Node.js runtime, symlinks has different edge cases that can bypass + // the permission model security guarantees. Thus, this API is disabled unless fs.read + // and fs.write permission has been given. + if (permission.isEnabled() && !permission.has('fs')) { + callback(new ERR_ACCESS_DENIED('fs.symlink API requires full fs.read and fs.write permissions.')); + return; } target = getValidatedPath(target, 'target'); @@ -1816,16 +1809,11 @@ function symlinkSync(target, path, type) { } } - if (permission.isEnabled()) { - // The permission model's security guarantees fall apart in the presence of - // relative symbolic links. Thus, we have to prevent their creation. - if (BufferIsBuffer(target)) { - if (!isAbsolute(BufferToString(target))) { - throw new ERR_ACCESS_DENIED('relative symbolic link target'); - } - } else if (typeof target !== 'string' || !isAbsolute(toPathIfFileURL(target))) { - throw new ERR_ACCESS_DENIED('relative symbolic link target'); - } + // Due to the nature of Node.js runtime, symlinks has different edge cases that can bypass + // the permission model security guarantees. Thus, this API is disabled unless fs.read + // and fs.write permission has been given. + if (permission.isEnabled() && !permission.has('fs')) { + throw new ERR_ACCESS_DENIED('fs.symlink API requires full fs.read and fs.write permissions.'); } target = getValidatedPath(target, 'target'); diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 459b9ad13..d96584bca 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -17,7 +17,6 @@ const { Symbol, Uint8Array, FunctionPrototypeBind, - uncurryThis, } = primordials; const { fs: constants } = internalBinding('constants'); @@ -31,8 +30,6 @@ const { const binding = internalBinding('fs'); const { Buffer } = require('buffer'); -const { isBuffer: BufferIsBuffer } = Buffer; -const BufferToString = uncurryThis(Buffer.prototype.toString); const { codes: { @@ -88,8 +85,6 @@ const { kValidateObjectAllowNullable, } = require('internal/validators'); const pathModule = require('path'); -const { isAbsolute } = pathModule; -const { toPathIfFileURL } = require('internal/url'); const { kEmptyObject, lazyDOMException, @@ -987,16 +982,11 @@ async function symlink(target, path, type_) { } } - if (permission.isEnabled()) { - // The permission model's security guarantees fall apart in the presence of - // relative symbolic links. Thus, we have to prevent their creation. - if (BufferIsBuffer(target)) { - if (!isAbsolute(BufferToString(target))) { - throw new ERR_ACCESS_DENIED('relative symbolic link target'); - } - } else if (typeof target !== 'string' || !isAbsolute(toPathIfFileURL(target))) { - throw new ERR_ACCESS_DENIED('relative symbolic link target'); - } + // Due to the nature of Node.js runtime, symlinks has different edge cases that can bypass + // the permission model security guarantees. Thus, this API is disabled unless fs.read + // and fs.write permission has been given. + if (permission.isEnabled() && !permission.has('fs')) { + throw new ERR_ACCESS_DENIED('fs.symlink API requires full fs.read and fs.write permissions.'); } target = getValidatedPath(target, 'target'); diff --git a/test/fixtures/permission/fs-symlink-target-write.js b/test/fixtures/permission/fs-symlink-target-write.js index c17d674d5..6e07bfa83 100644 --- a/test/fixtures/permission/fs-symlink-target-write.js +++ b/test/fixtures/permission/fs-symlink-target-write.js @@ -26,8 +26,7 @@ const writeOnlyFolder = process.env.WRITEONLYFOLDER; fs.symlinkSync(path.join(readOnlyFolder, 'file'), path.join(readWriteFolder, 'link-to-read-only'), 'file'); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', - permission: 'FileSystemWrite', - resource: path.toNamespacedPath(path.join(readOnlyFolder, 'file')), + message: 'fs.symlink API requires full fs.read and fs.write permissions.', })); assert.throws(() => { fs.linkSync(path.join(readOnlyFolder, 'file'), path.join(readWriteFolder, 'link-to-read-only')); @@ -37,18 +36,6 @@ const writeOnlyFolder = process.env.WRITEONLYFOLDER; resource: path.toNamespacedPath(path.join(readOnlyFolder, 'file')), })); - // App will be able to symlink to a writeOnlyFolder - fs.symlink(path.join(readWriteFolder, 'file'), path.join(writeOnlyFolder, 'link-to-read-write'), 'file', (err) => { - assert.ifError(err); - // App will won't be able to read the symlink - fs.readFile(path.join(writeOnlyFolder, 'link-to-read-write'), common.expectsError({ - code: 'ERR_ACCESS_DENIED', - permission: 'FileSystemRead', - })); - - // App will be able to write to the symlink - fs.writeFile(path.join(writeOnlyFolder, 'link-to-read-write'), 'some content', common.mustSucceed()); - }); fs.link(path.join(readWriteFolder, 'file'), path.join(writeOnlyFolder, 'link-to-read-write2'), (err) => { assert.ifError(err); // App will won't be able to read the link @@ -66,8 +53,7 @@ const writeOnlyFolder = process.env.WRITEONLYFOLDER; fs.symlinkSync(path.join(readWriteFolder, 'file'), path.join(readOnlyFolder, 'link-to-read-only'), 'file'); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', - permission: 'FileSystemWrite', - resource: path.toNamespacedPath(path.join(readOnlyFolder, 'link-to-read-only')), + message: 'fs.symlink API requires full fs.read and fs.write permissions.', })); assert.throws(() => { fs.linkSync(path.join(readWriteFolder, 'file'), path.join(readOnlyFolder, 'link-to-read-only')); diff --git a/test/fixtures/permission/fs-symlink.js b/test/fixtures/permission/fs-symlink.js index 4cf3b45f0..ba60f7811 100644 --- a/test/fixtures/permission/fs-symlink.js +++ b/test/fixtures/permission/fs-symlink.js @@ -54,7 +54,6 @@ const symlinkFromBlockedFile = process.env.EXISTINGSYMLINK; fs.readFileSync(blockedFile); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', - permission: 'FileSystemRead', })); assert.throws(() => { fs.appendFileSync(blockedFile, 'data'); @@ -68,7 +67,6 @@ const symlinkFromBlockedFile = process.env.EXISTINGSYMLINK; fs.symlinkSync(regularFile, blockedFolder + '/asdf', 'file'); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', - permission: 'FileSystemWrite', })); assert.throws(() => { fs.linkSync(regularFile, blockedFolder + '/asdf'); @@ -82,7 +80,6 @@ const symlinkFromBlockedFile = process.env.EXISTINGSYMLINK; fs.symlinkSync(blockedFile, path.join(__dirname, '/asdf'), 'file'); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', - permission: 'FileSystemRead', })); assert.throws(() => { fs.linkSync(blockedFile, path.join(__dirname, '/asdf')); @@ -90,4 +87,19 @@ const symlinkFromBlockedFile = process.env.EXISTINGSYMLINK; code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', })); +} + +// fs.symlink API is blocked by default +{ + assert.throws(() => { + fs.symlinkSync(regularFile, regularFile); + }, common.expectsError({ + message: 'fs.symlink API requires full fs.read and fs.write permissions.', + code: 'ERR_ACCESS_DENIED', + })); + + fs.symlink(regularFile, regularFile, common.expectsError({ + message: 'fs.symlink API requires full fs.read and fs.write permissions.', + code: 'ERR_ACCESS_DENIED', + })); } \ No newline at end of file diff --git a/test/parallel/test-permission-fs-symlink-relative.js b/test/parallel/test-permission-fs-symlink-relative.js index 4cc7d9205..9080f16c6 100644 --- a/test/parallel/test-permission-fs-symlink-relative.js +++ b/test/parallel/test-permission-fs-symlink-relative.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* --allow-fs-write=* +// Flags: --experimental-permission --allow-fs-read=* 'use strict'; const common = require('../common'); @@ -10,7 +10,7 @@ const { symlinkSync, symlink, promises: { symlink: symlinkAsync } } = require('f const error = { code: 'ERR_ACCESS_DENIED', - message: /relative symbolic link target/, + message: /symlink API requires full fs\.read and fs\.write permissions/, }; for (const targetString of ['a', './b/c', '../d', 'e/../f', 'C:drive-relative', 'ntfs:alternate']) { @@ -27,14 +27,14 @@ for (const targetString of ['a', './b/c', '../d', 'e/../f', 'C:drive-relative', } } -// Absolute should not throw +// Absolute should throw too for (const targetString of [path.resolve('.')]) { for (const target of [targetString, Buffer.from(targetString)]) { for (const path of [__filename]) { symlink(target, path, common.mustCall((err) => { assert(err); - assert.strictEqual(err.code, 'EEXIST'); - assert.match(err.message, /file already exists/); + assert.strictEqual(err.code, error.code); + assert.match(err.message, error.message); })); } } diff --git a/test/parallel/test-permission-fs-symlink.js b/test/parallel/test-permission-fs-symlink.js index c7d753c26..268a8ecb9 100644 --- a/test/parallel/test-permission-fs-symlink.js +++ b/test/parallel/test-permission-fs-symlink.js @@ -21,15 +21,26 @@ const commonPathWildcard = path.join(__filename, '../../common*'); const blockedFile = fixtures.path('permission', 'deny', 'protected-file.md'); const blockedFolder = tmpdir.resolve('subdirectory'); const symlinkFromBlockedFile = tmpdir.resolve('example-symlink.md'); +const allowedFolder = tmpdir.resolve('allowed-folder'); +const traversalSymlink = path.join(allowedFolder, 'deep1', 'deep2', 'deep3', 'gotcha'); { tmpdir.refresh(); fs.mkdirSync(blockedFolder); + // Create deep directory structure for path traversal test + fs.mkdirSync(allowedFolder); + fs.writeFileSync(path.resolve(allowedFolder, '../protected-file.md'), 'protected'); + fs.mkdirSync(path.join(allowedFolder, 'deep1')); + fs.mkdirSync(path.join(allowedFolder, 'deep1', 'deep2')); + fs.mkdirSync(path.join(allowedFolder, 'deep1', 'deep2', 'deep3')); } { // Symlink previously created + // fs.symlink API is allowed when full-read and full-write access fs.symlinkSync(blockedFile, symlinkFromBlockedFile); + // Create symlink for path traversal test - symlink points to parent directory + fs.symlinkSync(allowedFolder, traversalSymlink); } { @@ -38,6 +49,7 @@ const symlinkFromBlockedFile = tmpdir.resolve('example-symlink.md'); [ '--experimental-permission', `--allow-fs-read=${file}`, `--allow-fs-read=${commonPathWildcard}`, `--allow-fs-read=${symlinkFromBlockedFile}`, + `--allow-fs-read=${allowedFolder}`, `--allow-fs-write=${symlinkFromBlockedFile}`, file, ], @@ -47,6 +59,8 @@ const symlinkFromBlockedFile = tmpdir.resolve('example-symlink.md'); BLOCKEDFOLDER: blockedFolder, BLOCKEDFILE: blockedFile, EXISTINGSYMLINK: symlinkFromBlockedFile, + TRAVERSALSYMLINK: traversalSymlink, + ALLOWEDFOLDER: allowedFolder, }, } ); -- 2.30.2